feat!: Support publishing and consuming chain storage stream cells #5942

gibson042 · 2022-08-12T10:06:57Z

Description

Adds support for publishing and opportunistically consuming { height, values } chain storage stream cells in place of naked values, and switches the "published" chain storage subtree to use them.

I'm not sure how casting is intended to behave with batches, and confused about the committer.isValid() guard.

Security Considerations

There's a minor risk of stored data getting confused for a stream cell, but since everything is currently under our control, that should be fine until we can eliminate naked values (at least in the "published" subtree).

Documentation Considerations

I'm not sure how to document the new structure, which I'm calling a stream cell per https://github.com/Agoric/agoric-sdk/tree/mfig-vstream/golang/cosmos/x/vstream/spec .

Testing Considerations

It looks like there are no current tests for the Go aspects, and how to add some isn't clear.

dckc · 2022-08-12T19:08:13Z

scripts/get-flattened-publication.sh

+  .height as $dataHeight |
+
+  # Flatten each value independently.
+  .values[] | fromjson |


I wondered if this change had any impact on this script. Do we have any tests that would have failed if we didn't remember to update this script?

I'm pretty sure we don't have any tests of shell scripts, and I think that's fine. They're not exported from the packages and not part of the API of the package. They're just here to help organize the code.

It's perhaps not exported in the npm sense, but it's a customer-visible API (customer-required, in fact). I would like to see an integration test where

javascript code writes capData to a file

this script reads the capData file and writes flat data

we diff the flat data against expected results

Of course, I'd like to see a lot of things... I'm not sure whether it's cost-effective any time soon. It's a question of how often we're likely to tweak this script.

Perhaps it should be exported in the npm sense? that is: perhaps it should go in .bin?

The problem with a test like you describe above is that it's mostly navel-gazing—there's no connection between the hard-coded input and the kind of output that is expected of RPC nodes in the real world. But I do still thank that having something is a good idea.

OK, I've added shell-based testing. Any ideas for how to make sure they run with the rest of our tests?

The test nom script could call the shell test thing, maybe? (I haven't looked at the shell thing)

Added a simple runner at scripts/test/test.sh (and a GitHub workflow calling it).

turadg

Reviewed the chain JS. I don't know enough about the Go parts to review so not approving the PR.

If you'd like me to dive in enough to do a full review let's discuss because I'll have to reprioritize some stuff.

packages/casting/src/follower-cosmjs.js

turadg · 2022-08-12T19:32:13Z

packages/casting/test/fake-rpc-server.js

@@ -64,10 +64,19 @@ const fakeStatusResult = {
  },
 };

-export const startFakeServer = (t, fakeValues, marshaller = makeMarshal()) => {
+/** @typedef {Partial<import('ava').ExecutionContext<{cleanups: Array<() => void>}>> & {context}} fakeServerTestContext */


packages/casting/test/test-mvp.js

turadg · 2022-08-12T19:34:06Z

packages/vats/src/lib-chainStorage.js

@@ -38,11 +38,14 @@ harden(sanitizePathSegment);
 * @param {(message: StorageMessage) => any} handleStorageMessage a function for sending a storageMessage object to the storage implementation (cf. golang/cosmos/x/vstorage/vstorage.go)
 * @param {string} storeName currently limited to "swingset"


drive-by: type is effectively {'swingset'}

turadg · 2022-08-12T19:36:45Z

packages/vats/test/test-lib-chainStorage.js

-  // Instantiate chain storage over a simple in-memory implementation.
+// makeFakeChainStorageKit creates a chainStorage root node over an in-memory map
+// and exposes both the map and the sequence of received messages.
+const makeFakeChainStorageKit = (rootPath, options) => {


We have more than one kind of fake storage. How about makeTransientChainStorageKit? Not sure we even need "chain" #5945

Also, WDYT of this in lib-chainStorage for use by other packages?

turadg · 2022-08-12T19:36:55Z

packages/vats/test/test-lib-chainStorage.js

+// makeFakeChainStorageKit creates a chainStorage root node over an in-memory map
+// and exposes both the map and the sequence of received messages.


better as jsdoc. can include param types as well

turadg · 2022-08-12T19:39:55Z

scripts/get-flattened-publication.sh

+  .height as $dataHeight |
+
+  # Flatten each value independently.
+  .values[] | fromjson |


I'm pretty sure we don't have any tests of shell scripts, and I think that's fine. They're not exported from the packages and not part of the API of the package. They're just here to help organize the code.

kriskowal · 2022-08-12T21:36:02Z

golang/cosmos/x/vstorage/keeper/keeper.go

+	// but would add a requirement exclusive to AppendStorageValueAndNotify that its input be JSON.
+	// On the other hand, we could always extend this format in the future to include an indication
+	// that values are subject to a different encoding, e.g. `"valueEncoding":"base64"`.
+	Values []string `json:"values"`


I think string also limits cells to JSON (or at least text formats) in a way that []byte would not. I think we should commit to JSON. Strings might be better since interface{} has a lot more liberty to mess up 64 bit integers among other things.

I decided to stick with stream cell values being strings, aligning with the existing interfaces. So from the outside in we have

JS ChainStorageNode option "sequence" is false (default) vs. true.

JS ChainStorageNode method setValue(value: string) sends value with method "set" vs. "append".

vstorage.go invokes keeper.SetStorageAndNotify vs. keeper.AppendStorageValueAndNotify, in either case with the received value.

AppendStorageValueAndNotify loads or initializes a stream cell struct { Height string, Values []string }, appends the value, and calls SetStorageAndNotify with the cell's JSON serialization (this representation of a stream cell as JSON is the opinionated decision).

SetStorageAndNotify works down to SetStorage, which performs the actual write of value as a []byte with a static prefix.

kriskowal · 2022-08-12T21:43:09Z

golang/cosmos/x/vstorage/keeper/keeper.go

+	// otherwise initialize a blank cell.
+	currentData := k.GetData(ctx, path)
+	var cell StreamCell
+	_ = json.Unmarshal([]byte(currentData), &cell)


Error handling

Intentionally suppressed here because we populate cell manually if Unmarshal fails to do so.

kriskowal · 2022-08-12T21:45:11Z

packages/casting/src/follower-cosmjs.js

-        return;
+      let streamCell = decode(buf);
+      // Upgrade a naked value to a JSON stream cell if necessary.
+      if (!streamCell.height || !streamCell.values) {


Is 0 a valid height?

No, because it's not a string. And "0" is truthy.

kriskowal · 2022-08-12T21:55:39Z

packages/casting/src/follower-cosmjs.js

+          // eslint-disable-next-line no-continue
+          continue;
+        }
+        committer.commit({ value });


What’s the envelope around value for? I only see { value } getting committed, not { value, done }, the one likely suspect.

I haven't actually figured that out yet.

kriskowal · 2022-08-12T21:56:58Z

packages/casting/src/follower-cosmjs.js

+        if (!last) {
+          committer = prepareUpdateInOrder();
+        }


Why replace the committer only if final?

Only if not final. And I think replacing it in final position would make it look like there are gaps, although I could be wrong.

kriskowal · 2022-08-12T21:58:12Z

packages/casting/test/fake-rpc-server.js

-export const startFakeServer = (t, fakeValues, marshaller = makeMarshal()) => {
+/** @typedef {Partial<import('ava').ExecutionContext<{cleanups: Array<() => void>}>> & {context}} fakeServerTestContext */
+/**
+ * @param {fakeServerTestContext} t


Is there a reason for the type to have a little f?

No, I'll capitalize it.

kriskowal · 2022-08-12T22:00:36Z

packages/casting/test/fake-rpc-server.js

+/** @typedef {Partial<import('ava').ExecutionContext<{cleanups: Array<() => void>}>> & {context}} fakeServerTestContext */
+/**
+ * @param {fakeServerTestContext} t
+ * @param {Array<{any}>} fakeValues


What does {any} mean?

IIUC, it imposes no constraints beyond existence.

kriskowal · 2022-08-12T22:03:42Z

packages/casting/test/fake-rpc-server.js

  lastPort += 1;
  const PORT = lastPort;


Aside: this usually tells me we should listen on 0 and ask the kernel for the port we were assigned.

arirubinstein · 2022-08-14T01:37:19Z

@gibson042
I ran into an issue with the jq parse on vault managers:

./test.sh https://xnet.rpc.agoric.net published.vaultFactory.manager0.metrics                                                       5 ↵
jq: error (at <stdin>:16): null (null) cannot be matched, as it is not a string

gibson042 · 2022-08-14T20:36:15Z

@arirubinstein I think that was from iface missing in repeated references to the same slot, which should now be fixed. PTAL.

gibson042 · 2022-08-16T17:23:16Z

@arirubinstein I'd also like to rename blockHeight to responseBlockHeight, which would be a breaking change in the script output but provide more clarity in contrast with dataBlockHeight. Any objections?

Fixes #5366

…end"

…ells Fixes #5508

…-flattened-publication.sh Use existence of "blockHeight" and "values" properties rather than truthiness (which differs between JavaScript and `jq`).

…nseBlockHeight"

gibson042 added enhancement New feature or request javascript Pull requests that update Javascript code labels Aug 12, 2022

gibson042 marked this pull request as draft August 12, 2022 10:07

gibson042 changed the title ~~Gibson 5508 gap free chain storage~~ feat: Support publishing and opportunistically consuming { height, values } chain storage stream cells Aug 12, 2022

gibson042 changed the title ~~feat: Support publishing and opportunistically consuming { height, values } chain storage stream cells~~ feat: Support publishing and opportunistically consuming chain storage stream cells Aug 12, 2022

gibson042 changed the title ~~feat: Support publishing and opportunistically consuming chain storage stream cells~~ feat: Support publishing and consuming chain storage stream cells Aug 12, 2022

gibson042 changed the title ~~feat: Support publishing and consuming chain storage stream cells~~ feat!: Support publishing and consuming chain storage stream cells Aug 12, 2022

gibson042 marked this pull request as ready for review August 12, 2022 18:56

gibson042 requested review from michaelfig, turadg, dckc and kriskowal August 12, 2022 18:57

dckc reviewed Aug 12, 2022

View reviewed changes

turadg reviewed Aug 12, 2022

View reviewed changes

turadg mentioned this pull request Aug 12, 2022

test storage nodes data #5948

Merged

kriskowal approved these changes Aug 12, 2022

View reviewed changes

gibson042 force-pushed the gibson-5508-gap-free-chain-storage branch 2 times, most recently from e5f29c9 to 89daa67 Compare August 18, 2022 18:49

arirubinstein approved these changes Aug 19, 2022

View reviewed changes

gibson042 added the automerge:rebase Automatically rebase updates, then merge label Aug 20, 2022

gibson042 added 7 commits August 20, 2022 22:04

test(vats): Add tests for forthcoming chainStorage append messages

75b3e38

feat(vats): Add support for configuring chainStorage nodes as sequences

461204a

feat(casting): Update to consume stream cells

35db0da

Fixes #5366

feat(scripts): Update get-flattened-publication to consume stream cells

52de535

chore(vats): Update typedef to support new StorageMessage method "app…

6bf3878

…end"

chore(casting): Fix parameter types for fake RPC server

09e165c

feat(vats)!: Switch the "published" chain storage subtree to stream c…

a427c59

…ells Fixes #5508

gibson042 added 13 commits August 20, 2022 22:04

test(scripts): Add test files for get-flattened-publication.sh

ade09fb

fix(scripts): Fix flattening of falsy values

47fe9fa

style(casting): Accept review suggestions

da04888

refactor(scripts): Improve error reporting

0b97c21

test(scripts): Collapse DIFF_OPTS into DIFF

a167271

test(scripts): Add tests for repeated references of the same slot

38ce6ea

fix(scripts): Default slot iface from prior use

bf1f29a

test(scripts): Add a scripts test runner

c786022

chore: Commit to stream cell values being strings

929a07c

refactor: Rename stream cell "height" to "blockHeight"

40530fd

refactor(casting): Synchronize stream cell detection with scripts/get…

e4da515

…-flattened-publication.sh Use existence of "blockHeight" and "values" properties rather than truthiness (which differs between JavaScript and `jq`).

chore: Align with latest Tendermint RPC documentation

5328671

refactor: Rename get-flattened-publication.sh "blockHeight" to "respo…

2ce41da

…nseBlockHeight"

turadg force-pushed the gibson-5508-gap-free-chain-storage branch from 672526e to 2ce41da Compare August 20, 2022 22:04

mergify bot merged commit d8ede91 into master Aug 20, 2022

mergify bot deleted the gibson-5508-gap-free-chain-storage branch August 20, 2022 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Support publishing and consuming chain storage stream cells #5942

feat!: Support publishing and consuming chain storage stream cells #5942

gibson042 commented Aug 12, 2022 •

edited

Loading

dckc Aug 12, 2022 •

edited

Loading

turadg Aug 12, 2022

dckc Aug 12, 2022

dckc Aug 12, 2022

gibson042 Aug 12, 2022

gibson042 Aug 12, 2022

dckc Aug 13, 2022

gibson042 Aug 14, 2022

turadg left a comment

turadg Aug 12, 2022

turadg Aug 12, 2022

turadg Aug 12, 2022

turadg Aug 12, 2022

turadg Aug 12, 2022

turadg Aug 12, 2022

kriskowal Aug 12, 2022

gibson042 Aug 17, 2022 •

edited

Loading

kriskowal Aug 12, 2022

gibson042 Aug 13, 2022

kriskowal Aug 12, 2022

gibson042 Aug 13, 2022

kriskowal Aug 12, 2022

gibson042 Aug 13, 2022

kriskowal Aug 12, 2022

gibson042 Aug 13, 2022

kriskowal Aug 12, 2022

gibson042 Aug 13, 2022

kriskowal Aug 12, 2022

gibson042 Aug 13, 2022 •

edited

Loading

kriskowal Aug 12, 2022

arirubinstein commented Aug 14, 2022

gibson042 commented Aug 14, 2022

gibson042 commented Aug 16, 2022

		@@ -38,11 +38,14 @@ harden(sanitizePathSegment);
		* @param {(message: StorageMessage) => any} handleStorageMessage a function for sending a storageMessage object to the storage implementation (cf. golang/cosmos/x/vstorage/vstorage.go)
		* @param {string} storeName currently limited to "swingset"

		// makeFakeChainStorageKit creates a chainStorage root node over an in-memory map
		// and exposes both the map and the sequence of received messages.

feat!: Support publishing and consuming chain storage stream cells #5942

feat!: Support publishing and consuming chain storage stream cells #5942

Conversation

gibson042 commented Aug 12, 2022 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

dckc Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 Aug 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arirubinstein commented Aug 14, 2022

gibson042 commented Aug 14, 2022

gibson042 commented Aug 16, 2022

gibson042 commented Aug 12, 2022 •

edited

Loading

dckc Aug 12, 2022 •

edited

Loading

gibson042 Aug 17, 2022 •

edited

Loading

gibson042 Aug 13, 2022 •

edited

Loading